[code-scanning-fix] Fix workflow-graphql-id-unescaped: eliminate string interpolation in GraphQL mutations via variables#40757
Conversation
Wrap ownerId, projectId, and repositoryId with escapeGraphQLString() in the createProjectV2 and linkProjectV2ToRepository GraphQL mutations. Previously, title was consistently escaped but the node ID fields were not, creating a defense-in-depth gap. While GitHub API node IDs are opaque and currently safe, consistently applying escapeGraphQLString() ensures that if the values ever contain special characters the queries cannot be altered. Fixes code scanning alerts #627 and #628. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because no GitHub Actions runner was available. Make sure your repository has a runner available to run Copilot's review, or add a copilot-setup-steps.yml file specifying one with the runs-on attribute. See the docs for more details.
This PR addresses code-scanning warnings for potential GraphQL injection by ensuring GitHub node IDs interpolated into GraphQL mutations are escaped consistently.
Changes:
- Escape
ownerIdwhen building thecreateProjectV2GraphQL mutation. - Escape
projectIdandrepoIdwhen building thelinkProjectV2ToRepositoryGraphQL mutation.
Show a summary per file
| File | Description |
|---|---|
| pkg/cli/project_command.go | Escapes node ID inputs embedded into GraphQL mutation strings to close a defense-in-depth injection gap. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 1/1 changed files
- Comments generated: 2
| } | ||
| } | ||
| }`, projectId, repoId) | ||
| }`, escapeGraphQLString(projectId), escapeGraphQLString(repoId)) |
There was a problem hiding this comment.
Done in the latest commit. Both createProject and linkProjectToRepo (including its repo-ID query) now use static GraphQL query strings with variables passed as separate -f flags to gh api graphql. This eliminates all string interpolation from the query bodies, removing the need for manual escaping in these mutations entirely.
|
@copilot run pr-finisher skill |
|
Thanks for this security fix from the Code Scanning Fixer workflow! 🔒 The change is surgical and well-reasoned — wrapping One thing that would strengthen this PR:
If you'd like a hand adding that coverage, you can assign this prompt to your coding agent:
|
…ing interpolation Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Summary
Eliminates GraphQL string interpolation vulnerabilities (CWE-89 /
workflow-graphql-id-unescaped) inpkg/cli/project_command.goby converting all threegh api graphqlcall sites fromfmt.Sprintf-based query construction to fully parameterized GraphQL variables.Changes
pkg/cli/project_command.go— modified (high impact, non-breaking)Three call sites converted from inline string interpolation to GraphQL variables passed via
-fflags:createProjectV2mutationfmt.SprintfwithescapeGraphQLString(title)mutation($ownerId: ID!, $title: String!)+-f ownerId=... -f title=...repository(...)ID queryfmt.SprintfwithescapeGraphQLStringon both fieldsquery($owner: String!, $name: String!)+-f owner=... -f name=...linkProjectV2ToRepositorymutationfmt.Sprintfwith raw ID interpolationmutation($projectId: ID!, $repositoryId: ID!)+-f projectId=... -f repositoryId=...Commit
3c4b872acappliedescapeGraphQLString()to un-escaped node ID fields as a stopgap. Commitc969477edreplaces the entire interpolation approach with parameterization, eliminating the need forescapeGraphQLString()at these call sites.Security
workflow-graphql-id-unescapedcode scanning alerts (CWE-89)Testing notes
No new tests added. The parameterized calls are functionally equivalent to the previous interpolated calls for all well-formed inputs.
Checklist
pkg/cli/project_command.go)